test(storage): add emulator unit tests and cleanup #5453

vnghia · 2020-11-09T14:20:55Z

Since Bazel does not fully support pytest, I converted all those tests to unittest.

I have some questions

Should we convert all the code that are using testbench to emulator ?
When will we remove the testbench directory ?

Part of #4751

We finally reach a milestone in Complete GCS+gRPC plugin. From now, we could implementing GCS+gRPC without worrying about testing 🚀

This change is

coryan

Since Bazel does not fully support pytest, I converted all those tests to unittest.

Thanks.

I have some questions

Should we convert all the code that are using testbench to emulator ?

Seems to me all the google-cloud-cpp integration tests are already doing so. What else did you have in mind?

When will we remove the testbench directory ?

I think we should open a bug to track these things, but I believe we need to move and/or create a new README.md file like then one in the testbench/ directory.

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vnvo2409)

google/cloud/storage/emulator/BUILD, line 15 at r1 (raw file):

# limitations under the License.

py_test(

You need to add some boilerplate I think:

package(default_visibility = ["//visibility:public"])

licenses(["notice"])  # Apache 2.0

vnghia · 2020-11-09T15:45:06Z

The CI threw /usr/bin/env: 'python': No such file or directory. Do you have any idea why ? @coryan

I think it's because we have python3 in PATH but not python ? what do you think about symbolic link
ln -s /usr/bin/python3 /usr/bin/python

https://source.cloud.google.com/results/invocations/48148a79-4487-4d4e-90d0-30b8e3a1c1be/targets/cloud-cpp%2Fgithub%2Fgoogle-cloud-cpp%2Fmaster%2Fdocker%2Fintegration-presubmit/tests

Windows failed because of another reason: Those tests do not support to run on Windows. I have no idea why it happened 😕
https://source.cloud.google.com/results/invocations/5da479e1-b692-4f60-b7ad-96eb0e58111a/targets/cloud-devrel%2Fclient-libraries%2Fcpp%2Fgoogle-cloud-cpp%2Fwindows%2Fbazel-presubmit/log

coryan · 2020-11-09T16:21:36Z

Searching around I found this:

bazelbuild/bazel#8665

We do not install a plain "python" program in the docker images for these builds:

./ci/kokoro/docker/build.sh asan bash
bash-5.0$ python 
bash: python: command not found
bash-5.0$ python3 --version
Python 3.9.0

There is already an open bug for Bazel (bazelbuild/bazel#8446), but I think we will need to workaround it by installing a plain python program.

ghost · 2020-11-09T16:28:53Z

FYI Ubuntu only ships /usr/bin/python3 but not /usr/bin/python. On Fedora however, /usr/bin/python is symlinked to /usr/bin/python3. Focal has this package, but I'm not sure how safe the symlink would be on Bionic. WDYT?

coryan · 2020-11-09T16:35:22Z

This was Fedora:33, and I am not seeing the /usr/bin/python link:

docker run --rm -it fedora:33 bash
# dnf makecache && dnf install python3
# ls -l /usr/bin/py*
lrwxrwxrwx 1 root root     8 Oct  6 14:19 /usr/bin/pydoc -> ./pydoc3
lrwxrwxrwx 1 root root     8 Oct  6 14:19 /usr/bin/pydoc3 -> pydoc3.9
-rwxr-xr-x 1 root root    78 Oct  6 14:19 /usr/bin/pydoc3.9
lrwxrwxrwx 1 root root     9 Oct  6 14:19 /usr/bin/python3 -> python3.9
-rwxr-xr-x 1 root root 15536 Oct  6 14:20 /usr/bin/python3.9

Seems like we need to install python (aka python-unversioned-command) for Fedora:33:

# dnf makecache && dnf install python
# ls -l /usr/bin/py*
lrwxrwxrwx 1 root root     8 Oct  6 14:19 /usr/bin/pydoc -> ./pydoc3
lrwxrwxrwx 1 root root     8 Oct  6 14:19 /usr/bin/pydoc3 -> pydoc3.9
-rwxr-xr-x 1 root root    78 Oct  6 14:19 /usr/bin/pydoc3.9
lrwxrwxrwx 1 root root     9 Oct  6 14:46 /usr/bin/python -> ./python3
lrwxrwxrwx 1 root root     9 Oct  6 14:19 /usr/bin/python3 -> python3.9
-rwxr-xr-x 1 root root 15536 Oct  6 14:20 /usr/bin/python3.9

ghost · 2020-11-09T16:39:16Z

Windows failed because of another reason: Those tests do not support to run on Windows. I have no idea why it happened

It doesn't look like the python dependencies are being installed for the Windows builds. I think the Windows build only ran the integration tests (and not the testbench/emulator tests) but since you run bazel test inside run_integration_tests_emulator_bazel.sh they're getting picked up in the Windows build. Carlos can correct me on this.

Seems like we need to install python (aka python-unversioned-command) for Fedora:33:

Thanks. It should be installed by default, odd that the docker image omits it.

ghost · 2020-11-09T16:59:12Z

but since you run bazel test inside run_integration_tests_emulator_bazel.sh they're getting picked up in the Windows build.

Please disregard this. It made sense in my head when I typed it.

coryan · 2020-11-09T17:17:29Z

Windows failed because of another reason: Those tests do not support to run on Windows. I have no idea why it happened confused

We basically run bazel test ... on Windows (and Linux for that matter), so all the unit tests try to run.

Looks like we just need to install the pip dependencies and things would work, seems like an easy change in build.bat, but I really do not have the time to investigate all the details right now.

vnghia · 2020-11-10T15:45:31Z

We basically run bazel test ... on Windows (and Linux for that matter), so all the unit tests try to run.

I added tags=["manual"] to the BUILD file so the tests won't be caught by ...

The CI threw /usr/bin/env: 'python': No such file or directory.

Use RUN ln -s $(which python3) /usr/bin/python seems to be the easiest way.

I also deleted some redundant (grpcio) or unused (pytest) dependencies of python in the last commit.

coryan

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @vnvo2409)

ci/kokoro/docker/Dockerfile.fedora, line 26 at r2 (raw file):


# Link `python` to `python3`
RUN ln -s $(which python3) /usr/bin/python

I think we should simply install the python package. These Dockefiles are also good documentation to setup a development environment, and we should not be recommending something like this. Also, seems that this could fail unexpectedly in the future (say because the link is automatically created)

ci/kokoro/docker/Dockerfile.ubuntu, line 52 at r2 (raw file):


# Link `python` to `python3`
RUN ln -s $(which python3) /usr/bin/python

Ditto.

google/cloud/storage/ci/run_integration_tests_emulator_bazel.sh, line 34 at r2 (raw file):

# Run the unittests of the emulator before running integration tests.
"${BAZEL_BIN}" test "${bazel_test_args[@]}" "//google/cloud/storage/emulator:test_utils" \

Can we use :all and maybe pass --test_tag_filters=manual?

google/cloud/storage/emulator/BUILD, line 15 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

You need to add some boilerplate I think:
package(default_visibility = ["//visibility:public"])

licenses(["notice"])  # Apache 2.0

This is still missing I believe.

google/cloud/storage/emulator/BUILD, line 21 at r2 (raw file):

    imports = ["."],
    main = "tests/test_gcs.py",
    tags = ["manual"],

Can you open a bug to make these unit tests run on Windows? And add a TODO(#...) - $why here?

coryan · 2020-11-10T16:12:21Z

+kokoro:run

vnghia · 2020-11-10T16:23:08Z

Can you open a bug to make these unit tests run on Windows? And add a TODO(#...) - $why here?

These tests could run perfectly on Windows. However, I feel it's noop if we run the tests without running the emulator. WDYT ?

coryan · 2020-11-10T16:27:52Z

Can you open a bug to make these unit tests run on Windows? And add a TODO(#...) - $why here?

These tests could run perfectly on Windows. However, I feel it's noop if we run the tests without running the emulator. WDYT ?

Yes. I was thinking we would eventually run the Windows CI build against the emulator too.

codecov · 2020-11-10T16:28:35Z

Codecov Report

Merging #5453 (728104a) into master (4ab7ca2) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master    #5453    +/-   ##
========================================
  Coverage   95.49%   95.49%            
========================================
  Files        1057     1062     +5     
  Lines       97761    97984   +223     
========================================
+ Hits        93354    93573   +219     
- Misses       4407     4411     +4

Impacted Files	Coverage Δ
google/cloud/spanner/internal/spanner_stub.cc	`76.34% <0.00%> (-2.16%)`	⬇️
generator/generator.cc	`88.88% <0.00%> (-1.12%)`	⬇️
...cloud/pubsub/internal/subscription_session_test.cc	`98.97% <0.00%> (-1.03%)`	⬇️
google/cloud/pubsub/benchmarks/throughput.cc	`92.96% <0.00%> (-0.79%)`	⬇️
google/cloud/pubsub/internal/publisher_stub.cc	`96.15% <0.00%> (-0.21%)`	⬇️
.../spanner/benchmarks/multiple_rows_cpu_benchmark.cc	`91.80% <0.00%> (-0.11%)`	⬇️
google/cloud/completion_queue_test.cc	`96.85% <0.00%> (-0.10%)`	⬇️
...tor/integration_tests/golden/golden_client_test.cc	`98.17% <0.00%> (-0.08%)`	⬇️
google/cloud/pubsub/internal/subscriber_stub.cc	`98.61% <0.00%> (-0.06%)`	⬇️
google/cloud/spanner/samples/samples.cc	`84.62% <0.00%> (-0.06%)`	⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ab7ca2...728104a. Read the comment docs.

coryan

Looks good, let's see what happens with the builds +kokoro:run

Reviewed 8 of 8 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

google/cloud/storage/emulator/BUILD, line 21 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Can you open a bug to make these unit tests run on Windows? And add a TODO(#...) - $why here?

Oh cool, you fix that in this PR, thanks!

vnghia · 2020-11-11T11:56:44Z

kokoro/macos/bazel failed due to the same reason as Windows. I would like to ask if we have to rebuild the Dockerfile.* each kokoro build or not ? If yes, I think we should move

RUN pip3 install git+git://github.com/googleapis/python-storage@8cf6c62a96ba3fff7e5028d931231e28e5029f1c
RUN pip3 install grpcio==1.32.0 flask==1.1.2 pytest==6.0.2 httpbin==0.7.0 \
     scalpl==0.4.0 crc32c==2.1 gevent==20.9.0 gunicorn==20.0.4

to storage/ci/run_integration_tests_emulator_[bazel/cmake].sh. It has some benefits:

we don't have to rewrite these commands for each Dockerfile.*
the dependencies will be installed only when we run the storage integration tests.
fix kokoro/macos/bazel CI
save some quota/resources

I will move the unit tests of the emulator to storage/ci/run_integration_tests_emulator_bazel.sh so we don't have to run the unit tests without running the emulator ( which irritates me )

In addition, I think we should leave Window as-is and file an issue for activating the emulator and running the unit tests at the same time which makes more sense to me.

coryan

In addition, I think we should leave Window as-is and file an issue for activating the emulator and running the unit tests at the same time which makes more sense to me.

That SGTM.

I would like to ask if we have to rebuild the Dockerfile.* each kokoro build or not ?

Yes, but most builds use cached versions of the docker images and refresh/rebuild the image starting from that cache.

to storage/ci/run_integration_tests_emulator_[bazel/cmake].sh

I would prefer if we refactor these commands to something like

https://github.com/googleapis/google-cloud-cpp/blob/master/ci/install-cloud-sdk.sh

Reviewable status: complete! all files reviewed, all discussions resolved

vnghia · 2020-11-13T16:36:07Z

Yes, but most builds use cached versions of the docker images and refresh/rebuild the image starting from that cache.

I am a little bit confused: the dependencies from pip are cached with the Dockerfile.* or they will be reinstalled in every kokoro run ? If they are cached, I think installing they inside the Dockerfile.* are more efficient than installing them separately.

coryan · 2020-11-13T16:42:48Z

Yes, but most builds use cached versions of the docker images and refresh/rebuild the image starting from that cache.

I am a little bit confused: the dependencies from pip are cached with the Dockerfile.* or they will be reinstalled in every kokoro run ?

Yes 😁

The builds first download a (cached) docker image created from the Dockerfile.* used in that build, let say Dockerfile.fedora-install for this example. Then they run docker build -f Dockerfile.fedora-install, but use the downloaded image as a cache source.

If nothing has changed in Dockerfile.fedora-install then the cache satisfies the image and: (a) the docker build step is very fast (as it uses the cached image), and (b) the source of the docker image cache is more reliable than downloading N dependencies from M sources (in practice).

If there are changes in Dockerfile.fedora-install then some of the steps may still use the cache (which is nice), and the changes are validated as part of the PR (which is important).

The cached docker images are updated as part of the full builds post-PR, which are not required to be fast (though it is nice if they are).

vnghia · 2020-11-13T17:57:06Z

After a lot of trials, I think tags=["manual"] works best with our needs. Because:

Without the tag, these tests will run in all CI builds which use bazel ( it isn't what we want because these tests does not belong to the unit tests of the library ).
We can control when we run the tests, on which platform.

Can we use :all and maybe pass --test_tag_filters=manual?

It does not work, we have to write all the targets manually.

Even after #5498 is closed, we should continue using this tag since it allows us to run the tests only when we need

coryan

just waiting for the builds. This is great news.

Reviewed 1 of 3 files at r4, 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

coryan · 2020-11-13T19:48:02Z

Thanks again for this PR.

test(storage): add emulator unit tests and cleanup

c24ff30

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 9, 2020

product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Nov 9, 2020

silence the test log

f905c74

coryan marked this pull request as ready for review November 9, 2020 15:01

coryan requested a review from a team as a code owner November 9, 2020 15:01

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 9, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 9, 2020

coryan suggested changes Nov 9, 2020

View reviewed changes

address review

f353c80

coryan suggested changes Nov 10, 2020

View reviewed changes

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 10, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 10, 2020

address review

b2bd155

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 11, 2020

coryan reviewed Nov 11, 2020

View reviewed changes

yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 11, 2020

coryan reviewed Nov 12, 2020

View reviewed changes

fix build failure

c3ff9ab

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 13, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 13, 2020

fix unbound variable

728104a

vnghia requested a review from coryan November 13, 2020 18:46

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 13, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 13, 2020

coryan approved these changes Nov 13, 2020

View reviewed changes

coryan merged commit 2135c59 into googleapis:master Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(storage): add emulator unit tests and cleanup #5453

test(storage): add emulator unit tests and cleanup #5453

vnghia commented Nov 9, 2020 •

edited

Loading

coryan left a comment

vnghia commented Nov 9, 2020 •

edited

Loading

coryan commented Nov 9, 2020

ghost commented Nov 9, 2020

coryan commented Nov 9, 2020

ghost commented Nov 9, 2020

ghost commented Nov 9, 2020

coryan commented Nov 9, 2020

vnghia commented Nov 10, 2020

coryan left a comment

coryan commented Nov 10, 2020

vnghia commented Nov 10, 2020

coryan commented Nov 10, 2020

codecov bot commented Nov 10, 2020 •

edited

Loading

coryan left a comment

vnghia commented Nov 11, 2020 •

edited

Loading

coryan left a comment

vnghia commented Nov 13, 2020 •

edited

Loading

coryan commented Nov 13, 2020

vnghia commented Nov 13, 2020 •

edited

Loading

coryan left a comment

coryan commented Nov 13, 2020

test(storage): add emulator unit tests and cleanup #5453

test(storage): add emulator unit tests and cleanup #5453

Conversation

vnghia commented Nov 9, 2020 • edited Loading

coryan left a comment

Choose a reason for hiding this comment

vnghia commented Nov 9, 2020 • edited Loading

coryan commented Nov 9, 2020

ghost commented Nov 9, 2020

coryan commented Nov 9, 2020

ghost commented Nov 9, 2020

ghost commented Nov 9, 2020

coryan commented Nov 9, 2020

vnghia commented Nov 10, 2020

coryan left a comment

Choose a reason for hiding this comment

coryan commented Nov 10, 2020

vnghia commented Nov 10, 2020

coryan commented Nov 10, 2020

codecov bot commented Nov 10, 2020 • edited Loading

Codecov Report

coryan left a comment

Choose a reason for hiding this comment

vnghia commented Nov 11, 2020 • edited Loading

coryan left a comment

Choose a reason for hiding this comment

vnghia commented Nov 13, 2020 • edited Loading

coryan commented Nov 13, 2020

vnghia commented Nov 13, 2020 • edited Loading

coryan left a comment

Choose a reason for hiding this comment

coryan commented Nov 13, 2020

vnghia commented Nov 9, 2020 •

edited

Loading

vnghia commented Nov 9, 2020 •

edited

Loading

codecov bot commented Nov 10, 2020 •

edited

Loading

vnghia commented Nov 11, 2020 •

edited

Loading

vnghia commented Nov 13, 2020 •

edited

Loading

vnghia commented Nov 13, 2020 •

edited

Loading